-
-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
proposal: refactoring of options configuration #60
base: 2.14.x
Are you sure you want to change the base?
Conversation
9422cbf
to
10b71c0
Compare
10b71c0
to
208b1d7
Compare
Signed-off-by: codisart <[email protected]>
208b1d7
to
fb10f61
Compare
perhaps drop the trait, but rather add static methods to ValidatorOptionsObject for factory patterns. |
Hello @glensc, thank you for the review. I like your idea, what do you think of $options = ValidatorOptions::asArray(['min', 'max', 'inclusive'], func_get_args()); class ValidatorOptions
{
public static asArray(array $keys, array $args) {
$this->shouldHaveOnlyStringValues($keys);
$result = [];
foreach ($args as $arg) {
$result[array_shift($keys)] = $arg;
}
return $result;
}
private function shouldHaveOnlyStringValues(array $keys) : void
{
foreach ($keys as $key) {
if (! is_string($key)) {
throw new \InvalidArgumentException('The values of $keys should be only strings');
}
}
}
} |
@codisart Could you please create an RFC issue instead of a PR? |
Hello @boesing, I never did a RFC issue. Do you have an exemple to follow ? |
Description
This pull request is my proposal to refactor the options system for the validator objects.
I have two propositions.
The first one is to use a simple trait with a single public method to reduce the duplication of the code that transform function argumetns into an array of options
The second one is to use a full fleged object to do the transformation. It is in my opinion heavier to use but I think it could be the base of more refactoring of the options system. Maybe adding validation.